Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support input database without patient extensions #220

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

mikix
Copy link
Contributor

@mikix mikix commented Apr 23, 2024

Features:

  • The core__patient no longer requires the presence of race/ethnicity extensions to build (it now uses the deep-schema checking code)

Bug Fixes:

  • If there are both detailed and ombCategory values for an extension, no longer report both - just prefer the ombCategory version.

Checklist

  • Consider if documentation (like in docs/) needs to be updated
  • Consider if tests should be added
  • Update template repo if there are changes to study configuration

@mikix mikix force-pushed the mikix/patient-extensions-low-schema branch from 0d0b56e to 14780bc Compare April 23, 2024 19:49
PARTITION BY id, system
PARTITION BY id
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug fix, afaict - when you have both ombCategory and detailed, this was partitioning in such a way that both would have available_priority = 1 and both show up in the extension table. Then when you joined with patients, you'd get duplicate patient rows.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think that's a bug - it's left to the study author to determine the appropriate system for their use case. the core study preserves what it finds, and the user should use distinct() on patient IDs when they're counting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about this on slack - we think it is a bug, and will leave the code change in. But we have lots of questions about best approaches for race/ethnicity. Should race_detailed be a separate column? What to do with text? Problems for another day.

@mikix mikix force-pushed the mikix/patient-extensions-low-schema branch from 14780bc to 2f65227 Compare April 23, 2024 19:54
@mikix mikix marked this pull request as ready for review April 23, 2024 19:54
@mikix mikix force-pushed the mikix/patient-extensions-low-schema branch 5 times, most recently from 8533410 to 45da810 Compare April 24, 2024 17:05
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaict, this changed due to a re-run of the regression ETL on updated sample-bulk-fhir-datasets data (we inlined some medication codes in Aug '23)

@@ -153,7 +153,6 @@ CREATE TABLE core__documentreference AS
WITH temp_documentreference AS (
SELECT DISTINCT
dr.id,
dr.type,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there some dangling changes from a previous PR in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looks like I forgot to run generate-sql at the tail end of my deep-schema-support PR, after review changes.

PARTITION BY id, system
PARTITION BY id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think that's a bug - it's left to the study author to determine the appropriate system for their use case. the core study preserves what it finds, and the user should use distinct() on patient IDs when they're counting

cumulus_library/template_sql/sql_utils.py Show resolved Hide resolved
Features:
- The core__patient no longer requires the presence of race/ethnicity
  extensions to build (it now uses the deep-schema checking code)

Bug Fixes:
- If there are both detailed and ombCategory values for an extension,
  no longer report both - just prefer the ombCategory version.
@mikix mikix force-pushed the mikix/patient-extensions-low-schema branch from 45da810 to f278bba Compare April 24, 2024 19:09
@mikix mikix merged commit b6a79fe into main Apr 24, 2024
3 checks passed
@mikix mikix deleted the mikix/patient-extensions-low-schema branch April 24, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants